BUG 2075459: IOPS was not being set even when manually configured #5925

barbacbd · 2022-05-17T20:04:57Z

aws: IOPS was not being set even when manually configured

** Removed the terraform parameter that was constantly set to 0 unless this
was an io1 instance.

** Left the bootstrap terraform value remain as it isn't causing any issues, but
we should be aware if we want to change the boostrap aws instance to anything
other than gp3 then we will change that too.

** Added static value tests for the volume type, volume size and iops
for AWS.

openshift-ci · 2022-05-17T20:05:27Z

@barbacbd: This pull request references Bugzilla bug 2075459, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gpei@redhat.com), skipping review request.

Details

In response to this:

BUG 2075459: IOPS was not being set even when manually configured

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

barbacbd · 2022-05-17T20:05:50Z

The following link can be used to see where the static values came from.

pkg/types/aws/validation/machinepool.go

pkg/types/aws/validation/platform_test.go

r4f4 · 2022-05-19T19:38:40Z

pkg/types/aws/validation/machinepool.go

@@ -52,6 +50,40 @@ func ValidateMachinePool(platform *aws.Platform, p *aws.MachinePool, fldPath *fi
 	return allErrs
 }

+func validateVolumeSize(p *aws.MachinePool, fldPath *field.Path) field.ErrorList {


Nit: this function is so small/simple that it could maybe be integrated into the caller if.

r4f4 · 2022-05-19T19:41:54Z

/lgtm
I left some small comments but I'm nitpicking at this point.

r4f4 · 2022-05-19T20:50:12Z

/lgtm

patrickdillon

@barbacbd I believe you mentioned that there were some docs inconsistencies that needed to be corrected. Can you work with @mjpytlak (or ben scott, whose github username I don't know) to make these changes?

patrickdillon · 2022-05-25T18:48:42Z

pkg/types/aws/machinepool.go

@@ -78,8 +78,7 @@ func (a *MachinePool) Set(required *MachinePool) {

 // EC2RootVolume defines the storage for an ec2 instance.
 type EC2RootVolume struct {
-	// IOPS defines the amount of provisioned IOPS. This is only valid
-	// for type io1.
+	// IOPS defines the amount of provisioned IOPS. (KiB/s)


Suggested change

// IOPS defines the amount of provisioned IOPS. (KiB/s)

// IOPS defines the amount of provisioned IOPS. (KiB/s). IOPS may only be set for io1, io2, & gp3 volume types.

@barbacbd I'm available to work on this - what docs need revising?

https://docs.openshift.com/container-platform/4.10/storage/dynamic-provisioning.html
This is the first instance of the issue (not sure if there are more). In the section "AWS Elastic Block Store (EBS) object definition".

In #2 GP3 is a missing value.
In #3 It says that iops is only allowed for io1. It is allowed for gp3 and io2. It is required for io1 and io2 to be a value greater than 0.

https://docs.openshift.com/container-platform/4.10/storage/dynamic-provisioning.html This is the first instance of the issue (not sure if there are more). In the section "AWS Elastic Block Store (EBS) object definition".

In #2 GP3 is a missing value. In #3 It says that iops is only allowed for io1. It is allowed for gp3 and io2. It is required for io1 and io2 to be a value greater than 0.

@lpettyjo Brent has identified some docs inconsistencies in the storage area. Could you take a look? unsure if we should have a separate BZ created or how it should be handled. Thank you

@barbacbd I'd like to get this comment updated, because I don't think there's anywhere in the codebase that documents this behavior.

(And after the comment is updated the explain code will need to be regenerated.)

patrickdillon · 2022-06-03T01:35:38Z

pkg/types/aws/validation/machinepool.go

+	case "gp3":
+		// Terraform will accept any value


nit: even if Terraform accepts negative numbers, they don't make sense so we could enforce it here. It would be different than the io1, io2 case because 0 could potentially be valid...

** Removed the terraform parameter that was constantly set to 0 unless this was an io1 instance. ** Left the bootstrap terraform value remain as it isn't causing any issues, but we should be aware if we want to change the boostrap aws instance to anything other than gp3 then we will change that too. ** Added static value tests for the volume type, volume size and iops for AWS. aws: Static type checks fixed ** Adjusted the static type checks after reviewing the terraform pages. gp3 does not require any value for the iops, default will be set to 3000 io1 and io2 require a value (cannot be <0) sc1, st1, and gp2 require no value to be set (must be 0 iops) aws: remove used test data ** Reverting/Removing unused test data for the types/machines aws: Altered comments for machine ** Comments that are used to generate the docs were changed and the new comments and generated docs are added. aws: Fix error formatting ** Fixed capitalization and formatting in AWS types errors. ** Formatting Error messages. aws: IOPS change to GP3 volume types ** Updated comments/explain regenerated ** Added check for >= 0 IOPS for GP3, even though terraform will accept any value, we can go ahead and check it.

r4f4 · 2022-06-03T13:31:33Z

/lgtm

patrickdillon · 2022-06-03T15:47:11Z

/approve

patrickdillon · 2022-06-03T15:47:35Z

/retest

openshift-ci · 2022-06-03T15:53:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [patrickdillon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2022-06-03T17:43:39Z

/retest-required

Remaining retests: 2 against base HEAD 60ad1fe and 8 for PR HEAD d4a48a2 in total

openshift-ci · 2022-06-03T22:11:40Z

@barbacbd: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-libvirt	`d4a48a2`	link	false	`/test e2e-libvirt`
ci/prow/e2e-azure	`d4a48a2`	link	false	`/test e2e-azure`
ci/prow/e2e-crc	`d4a48a2`	link	false	`/test e2e-crc`
ci/prow/e2e-ibmcloud	`d4a48a2`	link	false	`/test e2e-ibmcloud`
ci/prow/e2e-openstack	`d4a48a2`	link	false	`/test e2e-openstack`
ci/prow/e2e-aws-single-node	`d4a48a2`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-metal-ipi	`d4a48a2`	link	false	`/test e2e-metal-ipi`
ci/prow/okd-e2e-aws	`d4a48a2`	link	false	`/test okd-e2e-aws`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci · 2022-06-03T22:13:18Z

@barbacbd: All pull requests linked via external trackers have merged:

openshift/installer#5925

Bugzilla bug 2075459 has been moved to the MODIFIED state.

Details

In response to this:

BUG 2075459: IOPS was not being set even when manually configured

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift/installer#5925 introduces checks which disallow our previous default of a gp2 volumeType with 100 IOPS as it was an incorrect configuration. From the linked change, IOPS may now only be set for io1, io2, & gp3 volume types.

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 17, 2022

openshift-ci bot requested review from jstuever and patrickdillon May 17, 2022 20:05

r4f4 reviewed May 18, 2022

View reviewed changes

pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved

barbacbd force-pushed the aws_iops_not_set branch 3 times, most recently from d56ac6a to 8a4a211 Compare May 19, 2022 13:20

rna-afk reviewed May 19, 2022

View reviewed changes

pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved

pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved

pkg/types/aws/validation/machinepool.go Outdated Show resolved Hide resolved

barbacbd force-pushed the aws_iops_not_set branch from 8a4a211 to a923d77 Compare May 19, 2022 19:16

barbacbd requested review from r4f4 and rna-afk May 19, 2022 19:17

r4f4 reviewed May 19, 2022

View reviewed changes

pkg/types/aws/validation/platform_test.go Outdated Show resolved Hide resolved

r4f4 reviewed May 19, 2022

View reviewed changes

openshift-ci bot assigned r4f4 May 19, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2022

barbacbd force-pushed the aws_iops_not_set branch from a923d77 to 0e507d1 Compare May 19, 2022 20:40

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 19, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2022

patrickdillon reviewed May 25, 2022

View reviewed changes

r4f4 removed their assignment Jun 2, 2022

patrickdillon reviewed Jun 3, 2022

View reviewed changes

barbacbd force-pushed the aws_iops_not_set branch from 0e507d1 to d4a48a2 Compare June 3, 2022 12:20

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2022

barbacbd requested a review from patrickdillon June 3, 2022 12:22

openshift-ci bot assigned r4f4 Jun 3, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2022

openshift-merge-robot merged commit 81fd6e4 into openshift:master Jun 3, 2022

abutcher mentioned this pull request Jun 22, 2022

Update default AWS volumeType chosen by hiveutil from gp2 to gp3. openshift/hive#1806

Merged

	// IOPS defines the amount of provisioned IOPS. (KiB/s)
	// IOPS defines the amount of provisioned IOPS. (KiB/s). IOPS may only be set for io1, io2, & gp3 volume types.

BUG 2075459: IOPS was not being set even when manually configured #5925

BUG 2075459: IOPS was not being set even when manually configured #5925

Uh oh!

Conversation

barbacbd commented May 17, 2022

Uh oh!

openshift-ci bot commented May 17, 2022

Uh oh!

barbacbd commented May 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r4f4 commented May 19, 2022

Uh oh!

r4f4 commented May 19, 2022

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r4f4 commented Jun 3, 2022

Uh oh!

patrickdillon commented Jun 3, 2022

Uh oh!

patrickdillon commented Jun 3, 2022

Uh oh!

openshift-ci bot commented Jun 3, 2022

Uh oh!

openshift-ci-robot commented Jun 3, 2022

Uh oh!

openshift-ci bot commented Jun 3, 2022

Uh oh!

openshift-ci bot commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants